HTTP: Warning and test fix #943

heifner · 2024-10-16T19:12:43Z

Fixes compiler warning about this not used by lambda
Add additional wait for no bytes in flight.
- On my machine, it takes awhile for the http_plugin to react to the close of the sockets and clear out the bytes in flight.
- Not sure why this requires such a time on my machine. Maybe WSL2 connections are really slow.

spoonincode · 2024-10-16T19:19:18Z

plugins/http_plugin/tests/unit_tests.cpp

@@ -606,6 +606,8 @@ BOOST_FIXTURE_TEST_CASE(bytes_in_flight, http_plugin_test_fixture) {
         req.set(http::field::host, "127.0.0.1:8891");
         boost::beast::http::write(s, req);
      }
+      //make sure got to the point http threads had responses queued


why does it matter if the response is queued or not at this point?

This avoids boost::beast::http::status::service_unavailable on my machine

maybe after the boost::beast::http::write(s, req) try doing a s.wait(wait_read), that will guarantee that the response is in flight. just a guess though

linh2931 · 2024-10-16T19:20:03Z

plugins/http_plugin/tests/unit_tests.cpp

@@ -606,6 +606,8 @@ BOOST_FIXTURE_TEST_CASE(bytes_in_flight, http_plugin_test_fixture) {
         req.set(http::field::host, "127.0.0.1:8891");
         boost::beast::http::write(s, req);
      }
+      //make sure got to the point http threads had responses queued
+      std::this_thread::sleep_for(std::chrono::seconds(5));


Why here sleeping 5 seconds but below sleeping 1 second?

Because the 1s is not for 4mb messages.

linh2931 · 2024-10-16T19:21:08Z

plugins/http_plugin/tests/unit_tests.cpp

   //now rip these connections out before the responses are completely sent
   connections.clear();
   //send some requests that should work still
   send_4mb_requests(8u);
   r = drain_http_replies();
+   for (const auto& e : r) {
+      ilog( "response: ${r}, count: ${c}", ("r", std::string(obsolete_reason(e.first)))("c", e.second));


I almost thought the r in ${r} was the r in r = drain_http_replies().

spoonincode

I don't think we should be adding sleeps to tests

spoonincode · 2024-10-16T20:31:17Z

plugins/http_plugin/tests/unit_tests.cpp

@@ -642,13 +644,14 @@ BOOST_FIXTURE_TEST_CASE(bytes_in_flight, http_plugin_test_fixture) {

   //load up some more requests that exceed max
   send_4mb_requests(32u);
-   //make sure got to the point http threads had responses queued
-   std::this_thread::sleep_for(std::chrono::seconds(1));
   //now rip these connections out before the responses are completely sent
   connections.clear();


If I understood your description right, this is the area that is problematic. Between this line and the line below the http plugin needs to have cleaned up all its ownstanding async_writes. Just closing the socket here does indeed send the FIN but the http threads need time to wake up and handle that error on the async_write to then clear out what's in flight.

I wonder if what might work is to shutdown each connection and then async_read on each one until getting confirmation the remote side disconnected via an error code.

That would likely work. I added a check for bytes_in_flight since that is really what we want to test anyway. We want to verify that bytes in flight goes to 0 once connections are closed.

plugins/http_plugin/tests/unit_tests.cpp

ericpassmore · 2024-10-16T21:23:20Z

Note:start
category: System Stability
component: HTTP
summary: Fix warnings and test.
Note:end

spoonincode · 2024-10-16T21:26:49Z

plugins/http_plugin/tests/unit_tests.cpp


   //8 requests to start with
   send_requests(8u);
   std::unordered_map<boost::beast::http::status, size_t> r = scan_http_replies();
   BOOST_REQUIRE_EQUAL(r[boost::beast::http::status::ok], 8u);
   connections.clear();
+   wait_for_no_bytes_in_flight();


this test case has to do with requests in flight, not bytes, so this feels mismatched but maybe it still serves enough of its purpose?

A requests in flight would be better and useful to verify in both cases. I'll add it.

spoonincode · 2024-10-16T21:28:32Z

plugins/http_plugin/tests/unit_tests.cpp

@@ -604,6 +604,7 @@ BOOST_FIXTURE_TEST_CASE(bytes_in_flight, http_plugin_test_fixture) {
         boost::beast::http::request<boost::beast::http::empty_body> req(boost::beast::http::verb::get, "/4megabyte", 11);
         req.keep_alive(true);
         req.set(http::field::host, "127.0.0.1:8891");
+         s.wait(boost::asio::ip::tcp::socket::wait_write);


What is the purpose of this and the wait_read below? Why would the socket not be writable here? And if it isn't writable why does it matter before a sync write is issued?

Not needed. I'll remove.

spoonincode · 2024-10-18T19:23:21Z

plugins/http_plugin/tests/unit_tests.cpp

@@ -622,6 +622,12 @@ BOOST_FIXTURE_TEST_CASE(bytes_in_flight, http_plugin_test_fixture) {
      return count_of_status_replies;
   };

+   auto wait_for_no_bytes_in_flight = [&](uint16_t max = std::numeric_limits<uint16_t>::max()) {
+      while (http_plugin->bytes_in_flight() > 0 && http_plugin->requests_in_flight() > 0 && --max)


When waiting for no bytes in flight why do we need to look at requests in flight too?

No "need", just thought it was good to also verify requests_in_flight appropriately hits 0.

spoonincode · 2024-10-18T19:25:37Z

plugins/http_plugin/tests/unit_tests.cpp

@@ -646,9 +652,13 @@ BOOST_FIXTURE_TEST_CASE(bytes_in_flight, http_plugin_test_fixture) {
   std::this_thread::sleep_for(std::chrono::seconds(1));


Can we get rid of this final sleep? It seems like waiting for each connection to indicate it's readable would suffice. But if we need to make another busy loop maybe we could wait to see requests_in_flight is 32

greg7mdp · 2024-10-20T01:33:36Z

plugins/http_plugin/tests/unit_tests.cpp

@@ -623,7 +623,7 @@ BOOST_FIXTURE_TEST_CASE(bytes_in_flight, http_plugin_test_fixture) {
   };

   auto wait_for_no_bytes_in_flight = [&](uint16_t max = std::numeric_limits<uint16_t>::max()) {
-      while (http_plugin->bytes_in_flight() > 0 && http_plugin->requests_in_flight() > 0 && --max)


Seems like this shouldn't make a difference as they should either both be 0 or both non-zero.

I'm confused by that too. And I don't understand the discrepancy why wait_for_no_bytes_in_flight looks at both while wait_for_no_requests_in_flight looks at only one. If the intent is to look at both then a common wait_for_nothing_in_flight could be made that looks at both.

In practice the requests in flight also matter. The issue is that bytes in flight can reach 0 before all the requests are done (or started).

bytes_in_flight is only incremented on sending the response. We do not, currently at least, include the request.

spring/plugins/http_plugin/include/eosio/http_plugin/beast_http_session.hpp

Lines 466 to 468 in 2b43faf

virtual void send_response(std::string&& json, unsigned int code) final {

auto payload_size = json.size();

increment_bytes_in_flight(payload_size);

Therefore, requests can be in progress, but bytes_in_flight can be 0. So we need to wait until all requests are complete to know that all bytes_in_flight are accounted for.

So could we just wait for requests_in_flight() to be 0, and then assert that bytes_in_flight() is 0 as well?

Sure, that sounds better

…th a comment on why

heifner added 2 commits October 16, 2024 14:10

Fix warning

dc3eaad

Update test for slow connections

d507054

heifner requested review from linh2931 and greg7mdp October 16, 2024 19:12

spoonincode reviewed Oct 16, 2024

View reviewed changes

linh2931 reviewed Oct 16, 2024

View reviewed changes

Use f instead of r to be less confusing

cfae058

spoonincode requested changes Oct 16, 2024

View reviewed changes

greg7mdp approved these changes Oct 16, 2024

View reviewed changes

heifner added the OCI Work exclusive to OCI team label Oct 16, 2024

spoonincode reviewed Oct 16, 2024

View reviewed changes

Add verification that bytes_in_flight is 0

5ceda4c

greg7mdp reviewed Oct 16, 2024

View reviewed changes

plugins/http_plugin/tests/unit_tests.cpp Show resolved Hide resolved

spoonincode reviewed Oct 16, 2024

View reviewed changes

heifner added 3 commits October 16, 2024 18:23

Remove unneeded wait()s

995da77

Add a requests_in_flight and verify in test

e8de934

Add back original sleep

98db2f4

linh2931 approved these changes Oct 17, 2024

View reviewed changes

greg7mdp approved these changes Oct 18, 2024

View reviewed changes

spoonincode reviewed Oct 18, 2024

View reviewed changes

heifner added 2 commits October 18, 2024 15:07

Replace sleep with a loop that waits for number of requests to be queued

f35f6f0

Fix while logic

2b43faf

greg7mdp reviewed Oct 20, 2024

View reviewed changes

spoonincode approved these changes Oct 21, 2024

View reviewed changes

Update wait_for_no_bytes_in_flight to check for requests_in_flight wi…

4a96d63

…th a comment on why

greg7mdp approved these changes Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP: Warning and test fix #943

HTTP: Warning and test fix #943

heifner commented Oct 16, 2024 •

edited

Loading

spoonincode Oct 16, 2024

heifner Oct 16, 2024

spoonincode Oct 16, 2024

linh2931 Oct 16, 2024

heifner Oct 16, 2024

linh2931 Oct 16, 2024

heifner Oct 16, 2024

spoonincode left a comment

spoonincode Oct 16, 2024

heifner Oct 16, 2024

ericpassmore commented Oct 16, 2024

spoonincode Oct 16, 2024

heifner Oct 16, 2024

spoonincode Oct 16, 2024

heifner Oct 16, 2024

spoonincode Oct 18, 2024

heifner Oct 18, 2024

spoonincode Oct 18, 2024

greg7mdp Oct 20, 2024 •

edited

Loading

spoonincode Oct 21, 2024

heifner Oct 21, 2024

greg7mdp Oct 21, 2024

heifner Oct 21, 2024

		@@ -646,9 +652,13 @@ BOOST_FIXTURE_TEST_CASE(bytes_in_flight, http_plugin_test_fixture) {
		std::this_thread::sleep_for(std::chrono::seconds(1));

	virtual void send_response(std::string&& json, unsigned int code) final {
	auto payload_size = json.size();
	increment_bytes_in_flight(payload_size);

HTTP: Warning and test fix #943

Are you sure you want to change the base?

HTTP: Warning and test fix #943

Conversation

heifner commented Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spoonincode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpassmore commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg7mdp Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heifner commented Oct 16, 2024 •

edited

Loading

greg7mdp Oct 20, 2024 •

edited

Loading